Fix client-v2: convert the read value to the POJO field's primitive type - #3000
Open
polyglotAI-bot wants to merge 1 commit into
Open
Fix client-v2: convert the read value to the POJO field's primitive type#3000polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
…type The compiled POJO setter left a value of the reader's own primitive type on the operand stack, so any column whose reader type differed from the setter parameter type produced a class that does not verify. The conversion is now emitted for every reader/field combination following Java narrowing and widening rules, and a primitive field bound to a column the reader decodes into a Number is unboxed instead of emitting a primitive class constant. Fixes: #2999
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #2999.
SerializerUtils.compilePOJOSettergenerates the bytecode that reads a column and calls the POJO's setter. On theoperand stack the value has the type of the reader method that produced it, which is frequently not the setter's
parameter type — and the conversion was only emitted for a few combinations (
longToOpcode/floatToOpcode/doubleToOpcodereturned-1forbyte/short/char/boolean, and theInt8/UInt8/Int16/Enum8/Enum16/Boolbranches computed no conversion at all). The generated class therefore failed verification(
java.lang.VerifyErrorwhen the setter class is first linked, i.e. on the first row read) for e.g. anInt64,UInt32,Float32,Float64orBFloat16column bound to abyte/short/char/booleanfield, and for anInt8/UInt8/Int16/Enum8/Enum16/Boolcolumn bound to along/float/doublefield.Separately, the generic fallback branch emitted
LDCof a class constant for the target type andCHECKCASTwith itsinternal name; for a primitive field that yields a class constant named
"I"/"J"and aCHECKCAST int, and passesthe
Objectreturned byreadValueto a primitive descriptor. SoInt128/UInt128/Int256/UInt256/Decimal*columns could not be read into a primitive field at all, even though their value is a
Number.Changes
SerializerUtils.binaryReaderMethodForTypenow records the reader's return type and delegates to a newemitPrimitiveConversion(mv, sourceType, targetType), which emits the full conversion for every pair followingJLS narrowing/widening rules (a wide value is narrowed to
intfirst, then tobyte/short/char;booleangoesthrough the new
numberToBoolean(double)because no opcode normalizes a number into 0/1). The now-unusedlongToOpcode/floatToOpcode/doubleToOpcodehelpers are removed.ClickHouseDataType.toPrimitiveType(...), whichmaps
chartoshortand so producedI2Srather thanI2Cfor acharfield.else if (targetType.isPrimitive())branch: the value is read with the wrapper class as the type hint andunboxed —
convertToBooleanfor abooleanfield, otherwiseobjectToNumber(new helper) plus the matchingNumber.xxxValue()(intValue()+I2Cforchar, asNumberhas nocharValue()). A column whose value is nota number now fails with a clear
IllegalArgumentExceptioninstead of invalid bytecode.CHANGELOG.mdanddocs/features.mdupdated.Not touched here (each is the subject of its own open PR on the same method, to keep the changes reviewable):
the
UInt64branch (#2997) and theNullablemarker for primitive fields (#2994).Test
SerializerUtilsTest.testCompiledSetterConvertsValueToPrimitiveFieldType— one@DataProvidercovering everycolumn type read into a primitive with all eight primitive field types, including narrowing (
Int64300 →byte44), unsigned readers (
UInt320xFFFFFFFF→short-1), saturation (Float641e30 →Long.MAX_VALUE), thelow-order-bits case (
Int1282^64+5 →long5), boolean 0/non-zero, and theNumber-valued column types(
Int128,UInt128,Int256,Decimal64). Rows that already worked before this change (e.g.Int32→byte,UInt16→long,Bool→boolean) are included as contrast cases and assert their unchanged results.SerializerUtilsTest.testCompiledSetterRejectsPrimitiveFieldForNonNumericColumn—Date/IPv4/String/UUIDinto a numeric primitive field fails with
IllegalArgumentException.SerializerUtilsTest.testCompiledSetterKeepsBoxedFieldValue— contrast case pinning that a boxed (Long) fieldstill goes through
readValueand is set as aLong.QueryTests.testQueryReadToPOJOWithPrimitiveFields(integration) — proves the fix on the live query path:queryAll(sql, Pojo.class, schema)withInt64→short,Float64→byte,Int128→long,Decimal64→double,Int64→boolean. This test fails withVerifyErroronmainand passes with the fix.Verified: 47 of the new data rows fail on
main(43VerifyError, 4 in the non-numeric test) and all pass with thefix;
mvn -pl client-v2 test→ 631 tests, 0 failures; the integration test above fails without the change.Pre-PR validation gate
main)client-v2unit suite greenAGENTS.md/docs/ai-review.md/docs/changes_checklist.mdCHANGELOG.mdentry added;docs/features.mdupdatedCompatibility
No public API change (
objectToNumberand thenumberToBoolean(double)overload are additions onSerializerUtils, called from the generated bytecode). Every column/field combination that worked before producesthe same value; the affected combinations previously threw
VerifyError, so no working behavior changes. Thecharfield conversion changes from
I2StoI2C, which matches the setter's descriptor.